OPRUN-4601: use resource-based RBAC for lifecycle-server auth#1290
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces controller-runtime auth wiring with a new ChangesLifecycle Server Authentication Middleware
Sequence DiagramsequenceDiagram
participant Client
participant AuthMiddleware as Auth Middleware
participant Authenticator as TokenReview Authenticator
participant Authorizer as SubjectAccessReview Authorizer
participant Handler as Inner Handler
Client->>AuthMiddleware: HTTP Request
AuthMiddleware->>Authenticator: AuthenticateRequest()
alt Authentication Error
Authenticator-->>AuthMiddleware: Error
AuthMiddleware-->>Client: 500 Internal Server Error
else Unauthenticated
Authenticator-->>AuthMiddleware: No User
AuthMiddleware-->>Client: 401 Unauthorized
else Authenticated
Authenticator-->>AuthMiddleware: User Identity
AuthMiddleware->>Authorizer: Authorize(user, verb="get", apiGroup="lifecycle.olm.openshift.io", resource="lifecycles")
alt Authorization Error
Authorizer-->>AuthMiddleware: Error
AuthMiddleware-->>Client: 500 Internal Server Error
else Denied or NoOpinion
Authorizer-->>AuthMiddleware: Denied / NoOpinion
AuthMiddleware-->>Client: 403 Forbidden
else Allowed
Authorizer-->>AuthMiddleware: Allowed
AuthMiddleware->>Handler: Forward request
Handler-->>Client: Handler response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
7fc0232 to
cd520b2
Compare
perdasilva
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@perdasilva: This pull request references OPRUN-4601 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/lifecycle-server/auth.go (1)
21-24: ⚡ Quick winKeep these constants unexported.
APIGroupandResourcelook internal to this middleware and the tests can still use them without exporting. Lowercasing them avoids accidentally growing theserverpackage API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/lifecycle-server/auth.go` around lines 21 - 24, Rename the exported constants APIGroup and Resource to unexported identifiers (e.g., apiGroup and resource) in the auth.go file and update all internal references to those symbols (including unit tests in the same package) so the package API surface doesn't grow unintentionally; ensure no external package code relies on APIGroup/Resource and adjust tests or move them into the same package if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/lifecycle-server/auth.go`:
- Around line 86-92: The SAR call is missing the resource Name so
resourceNames-scoped RBAC can't match; in the authz.Authorize invocation that
constructs authorizer.AttributesRecord (the block that sets User: res.User,
Verb: "get", APIGroup: APIGroup, Resource: Resource, ResourceRequest: true),
extract the package name from the request path (the {package} segment of
/api/{version}/lifecycles/{package} using req.URL.Path parsing or the router
vars) and set AttributesRecord.Name to that package string before calling
authz.Authorize so the authorization is evaluated against the specific lifecycle
resource.
---
Nitpick comments:
In `@pkg/lifecycle-server/auth.go`:
- Around line 21-24: Rename the exported constants APIGroup and Resource to
unexported identifiers (e.g., apiGroup and resource) in the auth.go file and
update all internal references to those symbols (including unit tests in the
same package) so the package API surface doesn't grow unintentionally; ensure no
external package code relies on APIGroup/Resource and adjust tests or move them
into the same package if necessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 269ceef6-45fa-480f-9ce0-030e8cb99009
📒 Files selected for processing (4)
cmd/lifecycle-server/start.gogo.modpkg/lifecycle-server/auth.gopkg/lifecycle-server/auth_test.go
|
PR description says:
But I don't see those in the changeset. iirc, the manifests directory is generated, and I had to trace where to put the new lifecycle-controller manifests to ensure the generation step puts them in the CVO manifests directory. |
sorry about that - I've updated the description. Those were either wrong or breadcrumbs for the client-side RBAC requirements |
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
cd520b2 to
11ba9c5
Compare
|
/retest |
|
/hold /lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
11ba9c5 to
8bef57e
Compare
|
/unhold I've addressed the comments =D |
openshift-ci
Bot
removed
the
do-not-merge/hold
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
8bef57e to
3560581
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/lifecycle-server/auth.go (1)
86-92: ⚡ Quick winConsider adding an explicit HTTP method check to the auth middleware for defense-in-depth.
Currently, the auth middleware hardcodes
Verb: "get"when authorizing requests. While all existing routes are registered with the explicit"GET /path"pattern (which causes Go'shttp.ServeMuxto automatically reject non-GET requests with 405 Method Not Allowed), adding an explicit method gate in the middleware would prevent a potential security gap if future routes are added without an explicit method prefix.♻️ Proposed hardening diff
return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method != http.MethodGet { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } + res, ok, err := authn.AuthenticateRequest(req) if err != nil { log.Error(err, "authentication failed") http.Error(w, "Authentication failed", http.StatusInternalServerError) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/lifecycle-server/auth.go` around lines 86 - 92, The middleware currently passes a hardcoded Verb: "get" to authz.Authorize; add an explicit HTTP method check in the auth middleware before calling authz.Authorize so the request's method is validated (e.g., ensure req.Method == "GET" or map other allowed methods) and reject invalid methods with an HTTP 405/403 as appropriate; update the call site that constructs authorizer.AttributesRecord (the block using res.User and Verb: "get") to derive Verb from req.Method (normalized/lowercased) or to gate by req.Method first, so authorization always matches the actual HTTP verb.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/lifecycle-server/auth.go`:
- Around line 86-92: The middleware currently passes a hardcoded Verb: "get" to
authz.Authorize; add an explicit HTTP method check in the auth middleware before
calling authz.Authorize so the request's method is validated (e.g., ensure
req.Method == "GET" or map other allowed methods) and reject invalid methods
with an HTTP 405/403 as appropriate; update the call site that constructs
authorizer.AttributesRecord (the block using res.User and Verb: "get") to derive
Verb from req.Method (normalized/lowercased) or to gate by req.Method first, so
authorization always matches the actual HTTP verb.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c6fc3eef-1b7b-4474-83fb-c786fae4927f
📒 Files selected for processing (4)
cmd/lifecycle-server/start.gogo.modpkg/lifecycle-server/auth.gopkg/lifecycle-server/auth_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/lifecycle-server/auth_test.go
fgiudici
left a comment
fgiudici
left a comment
There was a problem hiding this comment.
We are missing the manifest for the new RBAC needed for the new auth in the PR.
Is something to be addressed later/somewhere else?
Replace the controller-runtime metrics auth filter with a custom resource-based authorization middleware. The metrics filter performed nonResourceURL-based SubjectAccessReviews, which meant the default system:discovery ClusterRole (granting GET on /api/*) allowed any authenticated user to access the lifecycle-server API. The new middleware creates SubjectAccessReviews with ResourceAttributes (apiGroup: lifecycle.olm.openshift.io, resource: lifecycles, verb: get) so access requires an explicit ClusterRoleBinding to the new lifecycle-server-consumer ClusterRole. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
3560581 to
2b05ab2
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fgiudici, perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified bypass The lifecycle-server will be spawn by the lifecycle-controller, not yet merged, so this code cannot be really tested. |
openshift-ci-robot
added
the
verified
|
@fgiudici: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
openshift-merge-bot
Bot
merged commit 12c6652
into
openshift:main
4 participants
Summary
filters.WithAuthenticationAndAuthorization) with a custom resource-based authorization middleware for the lifecycle-server APIsystem:discoveryClusterRole (granting GET on/api/*) allowed any authenticated user to bypass authorizationResourceAttributes(apiGroup: lifecycle.olm.openshift.io,resource: lifecycles,verb: get) so access requires an explicit ClusterRoleBinding to the newlifecycle-server-consumerClusterRoleChanges
pkg/lifecycle-server/auth.go— New auth middleware usingDelegatingAuthenticatorConfig+DelegatingAuthorizerConfigwith resource-basedAttributesRecordpkg/lifecycle-server/auth_test.go— 7 unit tests covering all auth code paths (401, 403, 500, 200, DecisionNoOpinion, attributes verification, reason non-leakage)cmd/lifecycle-server/start.go— Swap metrics filter for newNewAuthFilterTest plan
go build ./cmd/lifecycle-server/...compilesgo test ./pkg/lifecycle-server/...— all tests passgo vet ./pkg/lifecycle-server/... ./cmd/lifecycle-server/...— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores